Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect JSONML from stack overflow exceptions caused by recursion #723

Merged
merged 5 commits into from
Feb 17, 2023

Conversation

TamasPergerDWP
Copy link
Contributor

@TamasPergerDWP TamasPergerDWP commented Feb 10, 2023

The fix follows the method used in fixing the same issue for XML in the previous PR:
Limit the XML nesting depth for CVE-2022-45688 #720

Additionally, fixing a minor issue with XMLParserConfiguration.clone() and amending some Javadoc.

Resolves #722

…ngDepth param.

Amend Javadoc for XML and XMLParserConfiguration classes.
Limit the XML nesting depth for CVE-2022-45688 when using the JsonML transform.
* Configuration object for the XML to JSONML parser. The configuration is immutable.
*/
@SuppressWarnings({""})
public class XMLtoJSONMLParserConfiguration {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for not re-using XMLParserConfiguration?
Either way, please don't expose any new APIs for JSONML besides the max nesting depth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason: not all options of XMLParserConfiguration is supported with JSONML parsing, so it seemed cleaner to create a new config class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new APIs for JSONML:

  • public static JSONObject toJSONObject(String string, XMLtoJSONMLParserConfiguration config)
  • public static JSONObject toJSONObject(XMLTokener x, XMLtoJSONMLParserConfiguration config)

These are both necessary to support max nesting depth and the keepStrings option in one function.
The alternative would be to add create 4 new toJSONObject functions that include the int maxNestingDepth parameter, on top of the existing 4 such functions - the potential parameter combinations doubling with a new optional parameter.

Is this acceptable?

Copy link
Contributor Author

@TamasPergerDWP TamasPergerDWP Feb 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that I did not add the max depth limit override option for the toJSONArray calls - await a fix soon.

Update - fixed now, a new commit was pushed.

…JSONML.toJSONArray(...) functions, to facilitate max nesting depth override.
@TamasPergerDWP
Copy link
Contributor Author

@stleary I have a fix for the JSONArray and JSONObject as well - by adding the depth limit to the JSONTokener class. (See my internal PR: TamasPergerDWP#1)
Three questions:

  1. Shall I add the fix to the current PR, or open a new one?
  2. JSONArray and JSONObject are used internally in the CDL, Cookie and CookieList classes. I introduced the maxDepthLimit optional parameter into those classes, too. Is that OK? Do those classes need additional tests for the maxDepthLimit behaviour?
  3. The JSONTokener class is the parent of XMLTokener - after introducing the max nesting depth feature in JSONTokener, we may simplify the solution that is in place for the XML parsing (unifying both approaches). Is this something we should look into?

@stleary
Copy link
Owner

stleary commented Feb 11, 2023

Let's work through the JSONML changes before starting JSONObject and JSONArray.
Please rename XMLtoJSONMLParserConfiguration to something simpler, like JSONMLParserConfiguration.
The new API methods for JSONML look OK. Code also looks OK, but still needs an in-depth review, working on it.

@TamasPergerDWP
Copy link
Contributor Author

Let's work through the JSONML changes before starting JSONObject and JSONArray.

Fair enough, I will leave the other changes for another day.

Please rename XMLtoJSONMLParserConfiguration to something simpler, like JSONMLParserConfiguration.

Done, ready for review.

@stleary
Copy link
Owner

stleary commented Feb 12, 2023

@TamasPergerDWP The code looks good, just one comment

@TamasPergerDWP
Copy link
Contributor Author

@TamasPergerDWP The code looks good, just one comment

@stleary Code amended as requested

@stleary
Copy link
Owner

stleary commented Feb 14, 2023

What problem does this code solve?
JSONML should have the same StackOverflow protection as XML.

Risks
Low

Changes to the API?
Some new API methods were added for those who don't want to use the default nesting depth.

Will this require a new release?
Yes

Should the documentation be updated?
No

Does it break the unit tests?
No

Was any code refactored in this commit?
Future refactoring will unite the parserconfig classes in a class hierarchy of some sort.

Review status
APPROVED

@stleary
Copy link
Owner

stleary commented Feb 14, 2023

Starting 3 day comment window.

@stleary stleary merged commit 1275f68 into stleary:master Feb 17, 2023
@stleary stleary changed the title JSONML should be protected from stack overflow exceptions caused by recursion, resolving #722 Protect JSONML from stack overflow exceptions caused by recursion Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSONObject/Array should be protected from stack overflow exceptions caused by recursion
2 participants